fix: force process exit when a worker thread can not be terminated - #9793
Closed
nflaig wants to merge 3 commits into
Closed
fix: force process exit when a worker thread can not be terminated#9793nflaig wants to merge 3 commits into
nflaig wants to merge 3 commits into
Conversation
`process.exit()` joins every worker via `stop_sub_worker_contexts()`, so a worker that can not be terminated blocks the exit until the process manager gives up, 60s under docker and longer elsewhere. Nothing in JS can preempt that, the main thread is inside `uv_thread_join` so no timer runs. Record the failed termination process wide, it is a property of the process rather than of any one core, and leave on our own terms once `BeaconNode.close()` has returned. Safe at that point, the state is archived and the db is closed. Note this only works if the process is not pid 1. The kernel discards signals sent to pid 1 that have no handler installed, including SIGKILL, so a container running the node as pid 1 needs an init (docker --init, tini) for the exit to be deliverable. It logs an error and falls through to the blocking `process.exit()` otherwise, which is no worse than the current behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
馃挕 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 624c336201
鈩癸笍 About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 馃憤.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| logger?.debug(`Worker thread failed to terminate in ${retryCount * retryMs}ms`); | ||
| someWorkerTerminationFailed = true; |
There was a problem hiding this comment.
If the worker misses the final retry deadline but terminates while the remaining BeaconNode.close() steps are running, this flag remains permanently true. The handler then sends itself SIGTERM even though no worker remains to block process.exit(), changing a successful shutdown from exit 0 to exit 143. Track workers that are currently unterminated, or clear the state when the existing termination subscription receives a late event.
Useful? React with 馃憤聽/ 馃憥.
Contributor
Performance Report鉁旓笍 no performance regression detected Full benchmark results
|
Addresses review feedback. If a worker misses the retry deadline but terminates while the rest of `BeaconNode.close()` runs, it can no longer block `process.exit()`, so forcing the exit would turn a clean shutdown into 143 for no reason. Count unterminated workers rather than latching a flag, so one terminating late does not clear the record for another that is genuinely stuck. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
22 tasks
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Closing this, the approach does not generalise. It only works when the node is not pid 1. The kernel discards signals sent to pid 1 that have no handler installed, including SIGKILL, so a container running the node directly as pid 1 cannot signal itself at all:
That covers systemd and init-enabled containers but silently does nothing for default docker and Kubernetes, which is most deployments. Making it work needs Worth recording for whoever looks at this next: this also explains the earlier attempt in #9790 that appeared to fail. The flag propagation was likely fine, the kill simply did nothing, and I misattributed it. Synchronous
The real fix remains the root cause, a libuv handle in the network worker that never closes so the thread spins in |
github-project-automation
Bot
moved this from In Progress
to Done
in Lodestar Team Coordination
Member
Author
|
I told my claude to close this, seems a bit too hacky to me. but giving up on debugging the root cause now |
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #9790. That one keeps a stuck network worker from costing us the state archive, this one keeps it from costing us a 60s shutdown.
process.exit()joins every worker thread viastop_sub_worker_contexts()->uv_thread_join(), so a worker that can not be terminated blocks the exit until the process manager gives up. Nothing in JS can preempt that, the main thread is inside C++ and the event loop is dead, so no timer runs. Captured live from a wedged process:A signal is the only lever.
terminateWorkerThreadnow records the failure process wide, and onceBeaconNode.close()has returned, the CLI leaves on its own terms. That point is safe, the finalized state is archived and the db is closed.process.exit(0)This only works if the process is not pid 1
The kernel discards signals sent to pid 1 that have no handler installed, including SIGKILL. A container running the node directly as pid 1 therefore can not signal itself at all. Verified:
docker run node -e 'process.kill(process.pid,"SIGKILL")'docker run --initMeasured end to end on a mainnet node, with an init so the node is not pid 1, and the failed-termination path forced:
and without an init the same build survives its own SIGTERM and SIGKILL.
So when the signal is not deliverable this logs an actionable error naming the requirement and falls through to the blocking
process.exit(), which is no worse than today's behaviour. Deployments that want the fast exit needdocker --init,init: truein compose, tini, or systemd (where the node is never pid 1).Worth considering as a follow up: warn at startup when
process.pid === 1, so operators learn about it before it matters rather than during a shutdown.Testing
3 clean shutdowns on the final build: exit 0 in 6.0-7.7s, force-exit branch correctly not taken. Forced-failure path: exit 143 in 6.7s with zero force kills. The stuck worker itself is intermittent (~10% of shutdowns) and unrelated to this change, root cause notes at https://gist.github.com/nflaig/5f41cfc50f38baf5046a034162943dc3
AI Assistance Disclosure
Investigation and patch developed with Claude Code, validated on a mainnet node as above.
馃 Generated with Claude Code